HYPERFLEET-978 - feat: Add PUT command for internal status endpoints (2/2)#44
Merged
openshift-merge-bot[bot] merged 1 commit intoMay 15, 2026
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR bumps the API version to 1.0.13 and changes internal adapter status ingestion from POST to PUT. Service interface docs, models, examples, CHANGELOG, README/CLAUDE, and generated OpenAPI/Swagger specs were updated: POST operations for cluster/nodepool statuses were removed and corresponding PUT operations documented as adapter-scoped idempotent upserts (keyed by adapter name) with aggregation into unified cluster/nodepool status. Several schema field descriptions now state timestamps update on PUT instead of POST. Sequence Diagram(s)sequenceDiagram
participant Adapter as Adapter (internal client)
participant StatusService as StatusesInternal API
participant Store as Status Store / Aggregator
Adapter->>StatusService: PUT /clusters/{cluster_id}/statuses or /clusters/{cluster_id}/nodepools/{nodepool_id}/statuses { adapter_name, status }
StatusService->>Store: Upsert status keyed by (cluster|nodepool) + adapter_name
Store-->>StatusService: Ack (created/updated)
StatusService-->>Adapter: 200/204 (idempotent upsert confirmation)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
ma-hill
force-pushed
the
HYPERFLEET-978
branch
2 times, most recently
from
b7acb0a to
21bfb59
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.tsp`:
- Line 23: The version in main.tsp was bumped to "1.0.13" but CHANGELOG.md was
not updated; add a new top-level section for 1.0.13 in CHANGELOG.md following
Keep a Changelog format with the version header (1.0.13) and date, and include
entries under the standard headings (Added, Changed, Fixed, Removed) as
appropriate for this release, plus update the comparison/links block at the
bottom to include the 1.0.13 tag comparison URL; ensure the version string
exactly matches "1.0.13" used in main.tsp so the files stay in sync.
- Line 23: The `@info` annotation uses "#{ ... }" interpolation syntax that CI's
version parser can't read; update the `@info` line to use plain object notation
(remove the "#" interpolation) so the version key is directly parseable (ensure
the symbol `@info` and the "version" property remain and that contact and license
are regular objects with name and url fields) so CI can extract "1.0.13".
In `@schemas/core/swagger.yaml`:
- Around line 697-703: The API contract was changed to PUT-only (operationId
putNodePoolStatuses) but dependent repos still expect POST; revert or
temporarily preserve a POST operation for nodepool status (e.g., add a POST
operationId postNodePoolStatuses that upserts like PUT) and coordinate updates
across openshift-hyperfleet/hyperfleet-api,
openshift-hyperfleet/hyperfleet-adapter, and openshift-hyperfleet/architecture
before removing POST; ensure both operationIds (putNodePoolStatuses and
postNodePoolStatuses) behave identically until all consumers are migrated, then
remove POST in a coordinated release.
- Around line 697-703: The swagger text is inconsistent: the generated schema
now uses operationId putNodePoolStatuses and describes a PUT/upsert but the
original source comments still say the adapter POSTs; update the original source
documentation/comment for the shared status schema (the one that generates
operationId putNodePoolStatuses and the adapter status path) to say PUT/upsert
instead of POST, then regenerate the artifacts so schemas/core/swagger.yaml is
rebuilt (do not edit the generated swagger.yaml directly). Ensure all other
occurrences that mention the adapter "POSTs" in the shared status schema are
corrected to reflect PUT/upsert before running the generation step.
In `@services/statuses-internal.tsp`:
- Around line 20-23: The change removes POST support for status writes by
exposing only putClusterStatuses; coordinate a compatibility plan instead of
enforcing PUT-only: restore or retain POST-backed endpoint behavior (or add a
forwarding POST handler that calls putClusterStatuses), mark POST as deprecated
in docs and API router, update adapter tests/configs and generated client
expectations to accept both, and add a migration note and timeline so consumers
(and tests/architecture docs) can adapt before removing POST entirely; ensure
references to putClusterStatuses and any POST handler are updated consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 8d8a89d0-691f-41a2-bab0-80c3e4a9f1ee
📒 Files selected for processing (9)
CLAUDE.mdREADME.mdmain.tspmodels/statuses/example_adapter_status.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/statuses-internal.tsp
| */ | ||
| @service(#{ title: "HyperFleet API" }) | ||
| @info(#{ version: "1.0.12", contact: #{ name: "HyperFleet Team" }, license: #{ name: "Apache 2.0" ,url: "https://www.apache.org/licenses/LICENSE-2.0"} }) | ||
| @info(#{ version: "1.0.13", contact: #{ name: "HyperFleet Team" }, license: #{ name: "Apache 2.0" ,url: "https://www.apache.org/licenses/LICENSE-2.0"} }) |
There was a problem hiding this comment.
Add the required CHANGELOG.md entry for 1.0.13
This PR bumps main.tsp version but does not include the required changelog update in the provided changes.
As per coding guidelines {main.tsp,CHANGELOG.md}: When bumping the version in main.tsp, always update CHANGELOG.md with the new version section, changes under Added/Changed/Fixed/Removed headings, and updated comparison links following Keep a Changelog format.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.tsp` at line 23, The version in main.tsp was bumped to "1.0.13" but
CHANGELOG.md was not updated; add a new top-level section for 1.0.13 in
CHANGELOG.md following Keep a Changelog format with the version header (1.0.13)
and date, and include entries under the standard headings (Added, Changed,
Fixed, Removed) as appropriate for this release, plus update the
comparison/links block at the bottom to include the 1.0.13 tag comparison URL;
ensure the version string exactly matches "1.0.13" used in main.tsp so the files
stay in sync.
@info line is currently breaking version extraction in CI
The pipeline is failing to parse main.tsp version from this line, which blocks validation/release workflows.
Suggested formatting adjustment
-@info(#{ version: "1.0.13", contact: #{ name: "HyperFleet Team" }, license: #{ name: "Apache 2.0" ,url: "https://www.apache.org/licenses/LICENSE-2.0"} })
+@info(#{
+ version: "1.0.13",
+ contact: #{
+ name: "HyperFleet Team",
+ },
+ license: #{
+ name: "Apache 2.0",
+ url: "https://www.apache.org/licenses/LICENSE-2.0",
+ },
+})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @info(#{ version: "1.0.13", contact: #{ name: "HyperFleet Team" }, license: #{ name: "Apache 2.0" ,url: "https://www.apache.org/licenses/LICENSE-2.0"} }) | |
| `@info`(#{ | |
| version: "1.0.13", | |
| contact: #{ | |
| name: "HyperFleet Team", | |
| }, | |
| license: #{ | |
| name: "Apache 2.0", | |
| url: "https://www.apache.org/licenses/LICENSE-2.0", | |
| }, | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@main.tsp` at line 23, The `@info` annotation uses "#{ ... }" interpolation
syntax that CI's version parser can't read; update the `@info` line to use plain
object notation (remove the "#" interpolation) so the version key is directly
parseable (ensure the symbol `@info` and the "version" property remain and that
contact and license are regular objects with name and url fields) so CI can
extract "1.0.13".
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
README.md (1)
113-124:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDocument (or gate) migration/backward-compat strategy before removing POST from the contract.
This PR switches internal status writes to PUT-only, but linked HyperFleet repos still contain POST-based adapter/docs/test flows. Without a staged migration or temporary compatibility period, consumers will break when this contract is consumed.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 113 - 124, Before removing POST, add a documented migration and compatibility gate: update README and release notes to describe the POST→PUT migration timeline and required adapter changes, and implement a temporary compatibility path in the API layer (e.g., in statuses-internal.tsp keep POST handlers that forward/convert to the new PUT logic or enable them behind an opt-in feature flag) so existing HyperFleet adapters continue working; also add integration tests exercising both POST and PUT behaviors and a clear deprecation notice in statuses.tsp/statuses-internal.tsp showing when POST will be removed.services/statuses-internal.tsp (1)
23-34:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftModel PUT upsert responses with both 201 (create) and 200 (update) status codes.
Both
putClusterStatusesandputNodePoolStatusesare documented as create-or-update operations (upserted by adapter name), but the response union only includesCreatedResponse, which maps to HTTP 201. A successful update should return 200 OK or 204 No Content, not 201. Without including an appropriate 200/204 response in the union, the generated OpenAPI spec will incorrectly describe all successful calls as resource creations.Update both operations to include a response option for the update case:
| (CreatedResponse & AdapterStatus) // for creates | { `@statusCode` statusCode: 200; `@body`: AdapterStatus; } // for updates | BadRequestResponse | NotFoundResponse | ConflictResponseAlso applies to:
putNodePoolStatuses(lines 46–62)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/statuses-internal.tsp` around lines 23 - 34, The PUT upsert operations putClusterStatuses and putNodePoolStatuses currently only declare CreatedResponse (201) for successful calls, but need to model both create and update outcomes; update each method's return union to include (CreatedResponse & AdapterStatus) for creates AND an additional union member representing a 200 response with an AdapterStatus body (e.g., a typed object with `@statusCode` 200 and `@body`: AdapterStatus) alongside the existing BadRequestResponse, NotFoundResponse, and ConflictResponse so the OpenAPI spec correctly shows create (201) and update (200) responses.
♻️ Duplicate comments (1)
main.tsp (1)
22-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFix TypeSpec decorator values to use value-object syntax (
#{}) to unblock compilation.Line 22 and Lines 23-33 currently pass model-expression objects where value objects are required, which matches the CI errors (
expect-value). Switch these decorator arguments back to#{ ... }.Suggested fix
-@service({ title: "HyperFleet API" }) -@info({ +@service(#{ title: "HyperFleet API" }) +@info(#{ version: "1.0.13", - contact: { + contact: #{ name: "HyperFleet Team", url: "https://github.com/openshift-hyperfleet" }, - license: { + license: #{ name: "Apache 2.0", url: "https://www.apache.org/licenses/LICENSE-2.0" } })#!/bin/bash # Verify TypeSpec decorator object syntax in main.tsp set -euo pipefail echo "Checking decorators in main.tsp that should use value objects (#{ ... }):" rg -n '^\s*@(?:service|info)\(\{' main.tsp || true echo echo "Expected after fix: no matches above."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.tsp` around lines 22 - 33, The `@service` and `@info` decorator arguments in main.tsp are currently using model-expression object syntax `{ ... }` which causes the TypeSpec `expect-value` compilation error; change those decorator argument objects to value-object syntax by replacing the `{ ... }` with `#{ ... }` for the `@service`({ title: "HyperFleet API" }) and `@info`({ version: "1.0.13", contact: { ... }, license: { ... } }) usages so they become `@service`(#{ ... }) and `@info`(#{ ... }) respectively to satisfy the compiler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@main.tsp`:
- Line 24: The version string "version: \"1.0.13\"" in the `@info` decorator was
only patched even though removing POST status endpoints is a breaking API
change; update that version to a breaking semver bump (e.g., change "1.0.13" to
"2.0.0") in the `@info` decorator in main.tsp so the release follows semantic
versioning per the project policy.
---
Outside diff comments:
In `@README.md`:
- Around line 113-124: Before removing POST, add a documented migration and
compatibility gate: update README and release notes to describe the POST→PUT
migration timeline and required adapter changes, and implement a temporary
compatibility path in the API layer (e.g., in statuses-internal.tsp keep POST
handlers that forward/convert to the new PUT logic or enable them behind an
opt-in feature flag) so existing HyperFleet adapters continue working; also add
integration tests exercising both POST and PUT behaviors and a clear deprecation
notice in statuses.tsp/statuses-internal.tsp showing when POST will be removed.
In `@services/statuses-internal.tsp`:
- Around line 23-34: The PUT upsert operations putClusterStatuses and
putNodePoolStatuses currently only declare CreatedResponse (201) for successful
calls, but need to model both create and update outcomes; update each method's
return union to include (CreatedResponse & AdapterStatus) for creates AND an
additional union member representing a 200 response with an AdapterStatus body
(e.g., a typed object with `@statusCode` 200 and `@body`: AdapterStatus) alongside
the existing BadRequestResponse, NotFoundResponse, and ConflictResponse so the
OpenAPI spec correctly shows create (201) and update (200) responses.
---
Duplicate comments:
In `@main.tsp`:
- Around line 22-33: The `@service` and `@info` decorator arguments in main.tsp are
currently using model-expression object syntax `{ ... }` which causes the
TypeSpec `expect-value` compilation error; change those decorator argument
objects to value-object syntax by replacing the `{ ... }` with `#{ ... }` for
the `@service`({ title: "HyperFleet API" }) and `@info`({ version: "1.0.13",
contact: { ... }, license: { ... } }) usages so they become `@service`(#{ ... })
and `@info`(#{ ... }) respectively to satisfy the compiler.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: bcb2c957-cc8a-47d7-b4b9-e7c785d41474
📒 Files selected for processing (12)
CHANGELOG.mdCLAUDE.mdREADME.mdmain.tspmodels/common/model.tspmodels/statuses/example_adapter_status.tspmodels/statuses/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/statuses-internal.tsp
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
main.tsp (2)
23-33:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winPipeline is still failing to parse version from
@infodecorator.The CI version extraction script cannot parse the
#{ }interpolation syntax. The multi-line formatting did not resolve the issue. The past review comment requesting removal of the#interpolation is still valid.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.tsp` around lines 23 - 33, The `@info` decorator uses Erlang-style #{ ... } map/interpolation which the CI parser can't parse; change the decorator to a plain JSON-like or simple key-value syntax so the version can be extracted—specifically replace the `@info` block that contains the version field (the current `@info` and its version: "#{...}" map) with a non-# interpolated form (remove all leading '#' characters and the #{ } map wrapper around the contact/license/version entries) so the version string is a top-level literal in the decorator.
24-24:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPatch version bump insufficient for breaking API change.
Removing POST status endpoints (
postClusterStatuses,postNodePoolStatuses) is a breaking API contract change. The version bump from1.0.12to1.0.13(patch level) does not follow semantic versioning for breaking changes. This should be a major or minor version bump (e.g.,2.0.0or1.1.0).Evidence from linked repositories confirms real consumer impact:
hyperfleet-apiintegration tests callPostClusterStatusesWithResponseandPostNodePoolStatusesWithResponsehyperfleet-adapterdocs/tests referencePOST /clusters/{id}/statusesarchitecturerepo documentsPOST /clusters/{id}/statusesin multiple places🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@main.tsp` at line 24, The version bump to "1.0.13" is incorrect for a breaking API change (removal of POST endpoints postClusterStatuses and postNodePoolStatuses); update the package version to a breaking-change appropriate semver (e.g., "1.1.0" or "2.0.0") in main.tsp and ensure any release metadata/changelog and downstream client SDKs/docs/tests referencing PostClusterStatusesWithResponse/PostNodePoolStatusesWithResponse are updated accordingly so consumers are signaled of the breaking change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 10-17: Update the 1.0.13 changelog entry (under "## [1.0.13] -
2026-05-13") to explicitly mark the POST removal as breaking: add a new "###
Breaking Changes" or "### Removed" subsection and list the removed POST
endpoints (/clusters/{cluster_id}/statuses and
/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses) plus a short migration
note instructing adapters to use the PUT endpoints with upsert semantics instead
of POST.
In `@main.tsp`:
- Line 24: The release bumped main.tsp to version "1.0.13" removed the public
POST operations postClusterStatuses and postNodePoolStatuses (endpoints
/clusters/{cluster_id}/statuses and
/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses) while only defining PUT
variants in statuses-internal.tsp, breaking existing clients and tests; fix by
restoring and exporting the POST operations in the public spec
(statuses.tsp/main.tsp) so postClusterStatuses and postNodePoolStatuses remain
available (optionally mark them deprecated), or alternatively export the PUT
endpoints to the public spec and coordinate client/test updates, ensuring
main.tsp imports the correct statuses file and that operationIds
postClusterStatuses/postNodePoolStatuses are preserved for backward
compatibility.
---
Duplicate comments:
In `@main.tsp`:
- Around line 23-33: The `@info` decorator uses Erlang-style #{ ... }
map/interpolation which the CI parser can't parse; change the decorator to a
plain JSON-like or simple key-value syntax so the version can be
extracted—specifically replace the `@info` block that contains the version field
(the current `@info` and its version: "#{...}" map) with a non-# interpolated form
(remove all leading '#' characters and the #{ } map wrapper around the
contact/license/version entries) so the version string is a top-level literal in
the decorator.
- Line 24: The version bump to "1.0.13" is incorrect for a breaking API change
(removal of POST endpoints postClusterStatuses and postNodePoolStatuses); update
the package version to a breaking-change appropriate semver (e.g., "1.1.0" or
"2.0.0") in main.tsp and ensure any release metadata/changelog and downstream
client SDKs/docs/tests referencing
PostClusterStatusesWithResponse/PostNodePoolStatusesWithResponse are updated
accordingly so consumers are signaled of the breaking change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 1b859bb7-290c-4246-9c51-6feadb900da5
📒 Files selected for processing (12)
CHANGELOG.mdCLAUDE.mdREADME.mdmain.tspmodels/common/model.tspmodels/statuses/example_adapter_status.tspmodels/statuses/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/statuses-internal.tsp
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/statuses-internal.tsp (1)
18-34:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPUT upserts should not advertise only
201 Created.Both operations are documented as create-or-update, but the success union only models a created response. When the adapter status already exists, the contract should expose an update success (
200or204) instead of implying that every successful PUT created a new record.Also applies to: 41-62
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@services/statuses-internal.tsp` around lines 18 - 34, The API signature for putClusterStatuses incorrectly only returns CreatedResponse for an upsert; update the return union of putClusterStatuses (and the similar operation at lines 41-62) to include an update-success response (e.g., OkResponse or NoContentResponse / 200 or 204) in addition to CreatedResponse so callers can distinguish create vs update; locate the method putClusterStatuses and the other upsert operation and add the appropriate success type to the union (e.g., (CreatedResponse | OkResponse | AdapterStatus) or (CreatedResponse & AdapterStatus) | OkResponse & AdapterStatus or CreatedResponse | NoContentResponse) to reflect both create and update outcomes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-20: The 1.0.13 changelog section currently uses a non-standard
heading "### Breaking Changes"; update the 1.0.13 section so its notes follow
the repository's required Keep-a-Changelog headings: move any endpoint removal
notes currently under "### Breaking Changes" into a "### Removed" subsection and
move behavior/migration wording into a "### Changed" subsection, ensuring the
section contains "### Added", "### Changed", "### Fixed", and "### Removed"
headings and that the top-level version header for 1.0.13 remains intact.
In `@CLAUDE.md`:
- Line 49: The change removed POST endpoints which breaks clients and tests that
still use POST (e.g., OpenAPI operationId "postClusterStatuses",
client.PostClusterStatusesWithResponse, client.PostNodePoolStatusesWithResponse,
and handlers expecting POST for POST /clusters/{id}/statuses); either restore
POST endpoints as deprecated shims that forward to the new PUT handlers
(returning a deprecation warning/header and supporting the same payload), or
coordinate updates across repos by 1) keeping POST in the OpenAPI spec and
server handlers while adding the new PUT operation, 2) updating generated
clients and replacing calls to PostClusterStatuses/PostNodePoolStatuses with the
new PUT methods in integration tests and adapter code, and 3) updating
README/docs/architecture notes to document the migration and deprecation
timeline so all consumers can be updated atomically.
In `@models/statuses/example_adapter_status.tsp`:
- Line 57: The change to a PUT-only status API in example_adapter_status.tsp
will break existing consumers that call POST endpoints; add a compatibility plan
and implementation before merging: restore temporary POST handlers or redirects
for PostClusterStatusesWithResponse and PostNodePoolStatusesWithResponse (or
provide a compatibility shim) and update adapter docs/tests and architecture
docs to reflect the versioned rollout; include a note in the repo explaining the
deprecation timeline and required client changes, and validate the final plan
against the HyperFleet architecture standards referenced in the architecture
repository.
In `@models/statuses/model.tsp`:
- Line 88: Summary: Removing POST endpoints is a breaking change for consumers;
restore backward compatibility and provide a migration path. Fix: in
services/statuses.tsp (and any route/controller that removed POST), reintroduce
the POST handlers as wrappers that accept the existing POST contract and
internally forward to the new PUT logic (or call the same service method used by
the new PUT handlers), emit a deprecation warning header/log, and keep the
documentation change but add a clear deprecation note; specifically ensure
compatibility for PostClusterStatusesWithResponse and
PostNodePoolStatusesWithResponse by restoring their POST routes or adding
POST-to-PUT proxy handlers, update integration test mocks to accept both POST
and PUT, and add a short migration note in the status docs directing consumers
to regenerate clients and switch to PUT.
In `@README.md`:
- Line 113: Update the README to add a clear migration/compatibility section
describing the POST→PUT rollout: state that PUT is now the required write method
(mention "PUT-only writes"), specify the migration scope and a cutoff version
(e.g., consumers must be upgraded to vX.Y by date/epoch), list concrete consumer
changes (replace POST status ingestion calls with PUT to the /status or /ingest
endpoint, update client libraries and CI tests), and explicitly say whether POST
compatibility is retained temporarily (and where it is implemented if so, e.g.,
in HyperFleet gateway or backward-compat layer) so consumers know where to look;
place this under or next to the "Status ingestion" / "Write API" section and
reference HyperFleet architecture links for detailed expectations.
In `@schemas/core/openapi.yaml`:
- Line 4: The OpenAPI file shows a patch version (version: 1.0.13) but the PR
removes the breaking operations postClusterStatuses and postNodePoolStatuses;
update the API version to a breaking release (e.g., bump to 2.0.0 or an agreed
major version per your semver policy) instead of a patch, or alternatively
restore POST compatibility for postClusterStatuses/postNodePoolStatuses until
migration completes; ensure the version field in schemas/core/openapi.yaml is
updated accordingly and validate the change against the HyperFleet architecture
standards referenced in your guidelines.
---
Outside diff comments:
In `@services/statuses-internal.tsp`:
- Around line 18-34: The API signature for putClusterStatuses incorrectly only
returns CreatedResponse for an upsert; update the return union of
putClusterStatuses (and the similar operation at lines 41-62) to include an
update-success response (e.g., OkResponse or NoContentResponse / 200 or 204) in
addition to CreatedResponse so callers can distinguish create vs update; locate
the method putClusterStatuses and the other upsert operation and add the
appropriate success type to the union (e.g., (CreatedResponse | OkResponse |
AdapterStatus) or (CreatedResponse & AdapterStatus) | OkResponse & AdapterStatus
or CreatedResponse | NoContentResponse) to reflect both create and update
outcomes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: b3b19482-8162-4176-8f19-36df0c076c96
📒 Files selected for processing (12)
CHANGELOG.mdCLAUDE.mdREADME.mdmain.tspmodels/common/model.tspmodels/statuses/example_adapter_status.tspmodels/statuses/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/statuses-internal.tsp
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
schemas/core/openapi.yaml (1)
513-530:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdvertise an update success code for these PUT upserts.
Both operations now document create-or-update semantics, but the spec still only returns
201 Created. For an existing adapter entry, a successful PUT should be modeled as200/204(or document both200and201), otherwise the contract contradicts the idempotent upsert behavior and generated clients will miss the normal update path. Fix this in the source TSP and regenerate the artifacts.As per coding guidelines, "
{schemas,tsp-output}/**: Do not modify generated files in schemas/ or tsp-output/ directories directly" and "ensure the contract’s intent is idempotent adapter-scoped upsert".Also applies to: 592-603
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@schemas/core/openapi.yaml` around lines 513 - 530, The OpenAPI response for the PUT operation with summary "Adapter creates or updates nodepool status" currently only lists '201' despite upsert semantics; update the source TSP (not generated schemas/) for this operation to include a success response for updates (add '200' with a response schema or '204' no-content) in addition to '201' for creates, and apply the same change to the other PUT block referenced (lines ~592-603), then regenerate the artifacts so clients see both create and update success codes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-20: Remove the duplicate removal sentence that repeats the
endpoints `/clusters/{cluster_id}/statuses` and
`/clusters/{cluster_id}/nodepools/{nodepool_id}/statuses` from the "### Changed"
section and leave that removal only under the "### Removed" heading; also insert
a blank line immediately after the "### Removed" heading to follow markdown
conventions so the section renders properly.
---
Outside diff comments:
In `@schemas/core/openapi.yaml`:
- Around line 513-530: The OpenAPI response for the PUT operation with summary
"Adapter creates or updates nodepool status" currently only lists '201' despite
upsert semantics; update the source TSP (not generated schemas/) for this
operation to include a success response for updates (add '200' with a response
schema or '204' no-content) in addition to '201' for creates, and apply the same
change to the other PUT block referenced (lines ~592-603), then regenerate the
artifacts so clients see both create and update success codes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 67e46212-1519-4965-89c7-e93a3e984e8e
📒 Files selected for processing (12)
CHANGELOG.mdCLAUDE.mdREADME.mdmain.tspmodels/common/model.tspmodels/statuses/example_adapter_status.tspmodels/statuses/model.tspschemas/core/openapi.yamlschemas/core/swagger.yamlschemas/gcp/openapi.yamlschemas/gcp/swagger.yamlservices/statuses-internal.tsp
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pnguyen44 The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
Bot
merged commit dc7687c
into
openshift-hyperfleet:main
3 checks passed
Contributor
Author
|
/unhold |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change removes POST endpoints from the internal status API,
standardizing on PUT operations for idempotent status updates. This
simplifies the API surface and reduces redundancy in the OpenAPI
specifications.
Changes made:
statuses-internal.tspfor both cluster andnodepool status endpoints
Test Plan
Jira Issue